Skip to content

trident: promote rollback to stable v1; adjust rollback return type to detect no-servicing - #729

Open
bfjelds wants to merge 1 commit into
mainfrom
user/bfjelds/rollback-grpc-promotion
Open

trident: promote rollback to stable v1; adjust rollback return type to detect no-servicing#729
bfjelds wants to merge 1 commit into
mainfrom
user/bfjelds/rollback-grpc-promotion

Conversation

@bfjelds

@bfjelds bfjelds commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

Promotes manual-rollback's Rollback/RollbackStage/RollbackFinalize from trident.v1preview.RollbackService to the stable trident.v1.RollbackService.

Align manual rollback's no-op reporting with update()/install(): a no-op rollback (empty rollback chain, or host not in a rollback-eligible state) now reports (ExitKind::Done, ServicingType::NoActiveServicing) instead of a bare ExitKind::Done.

Context

This is the first step in enabling trident-acl-agent to run updates and rollbacks. Related PRs:

Validation

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

@bfjelds bfjelds changed the title trident: promote RollbackService to stable v1; align rollback return type with update/install trident: promote rollback to stable v1; adjust rollback return type to detect no-servicing Aug 5, 2026
…type with update/install

Two related API changes to tridentd's manual-rollback gRPC surface:

1. Promotes Rollback, RollbackStage, and RollbackFinalize from
   trident.v1preview.RollbackService to the stable trident.v1.RollbackService
   (proto/trident/v1/rollback_service.proto, new file). This gives rollback
   the same stable, caller-handles-reboot gRPC contract that
   UpdateStage/UpdateFinalize already have, instead of CLI-shelling.
   CheckRollback, GetRollbackChain, and GetRollbackTarget remain preview-only
   (see proto/trident/v1preview/rollback_service.proto).

2. Aligns manual rollback's no-op reporting with update()/install():
   - engine::manual_rollback::execute_rollback() and Trident::rollback() now
     return (ExitKind, ServicingType) instead of a bare ExitKind. A no-op
     (empty rollback chain, or host not in a rollback-eligible servicing
     state) now reports (ExitKind::Done, ServicingType::NoActiveServicing)
     rather than a bare ExitKind::Done indistinguishable from a real
     rollback at the gRPC layer.
   - The three gRPC handlers in services/rollback.rs now populate
     ServicingResponse.servicing_kind from that value, the same way
     update.rs/install.rs already do, instead of hardcoding None. This lets
     callers detect a no-op rollback from the response itself rather than
     needing a separate precondition query.
   - CheckRollback (which existed to answer that same "is a rollback
     available" question via a separate round-trip) is demoted back to
     trident.v1preview accordingly - its one caller no longer needs it.
     try_acquire_read_lock/reading_request/trident_error_to_status (and
     their Code/ErrorKind/OwnedRwLockReadGuard imports) are re-gated behind
     the grpc-preview feature, since check_rollback was their last
     non-preview caller.
   - main.rs's CLI `trident rollback` command updated for the new return
     shape (matches Install/Update/Commit's existing pattern).

Split out from user/bfjelds/acl-agent-rollback-grpc for isolated API
review: this covers only the tridentd-side proto/engine/server surface,
not trident-acl-agent's consumption of it.

Verified: cargo test -p trident --lib manual_rollback (22 passed), cargo
clippy -p trident --tests -- -D warnings (clean, default features), cargo
clippy -p trident --tests --all-features -- -D warnings (clean, confirms
grpc-preview-off build has no dead code from the CheckRollback demotion),
cargo fmt --check (clean).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86
@bfjelds
bfjelds force-pushed the user/bfjelds/rollback-grpc-promotion branch from 057a7ae to d27e576 Compare August 5, 2026 20:30
@bfjelds
bfjelds requested a lite review from Copilot August 5, 2026 20:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new stable v1 rollback() gRPC handler uses a partially-moved request (req.finalize moved out but req.kind() used later), which will fail to compile.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR promotes the core manual rollback RPCs (Rollback/RollbackStage/RollbackFinalize) into the stable trident.v1.RollbackService proto surface, while keeping preview-only rollback query RPCs in trident.v1preview. It also changes Trident’s internal rollback API to return (ExitKind, ServicingType) so gRPC callers can reliably distinguish a true rollback from a no-op via servicing_kind.

Changes:

  • Move rollback execution RPCs into a new stable proto/trident/v1/rollback_service.proto and trim v1preview to query-only rollback RPCs.
  • Wire up the stable v1 RollbackService implementation in the tridentd server and register it unconditionally.
  • Change Trident::rollback / manual_rollback::execute_rollback to return (ExitKind, ServicingType) and map no-op rollback to ServicingType::NoActiveServicing.
File summaries
File Description
proto/trident/v1preview/rollback_service.proto Removes execution RPCs and keeps preview-only rollback queries; references stable v1 ManualRollbackKind.
proto/trident/v1/rollback_service.proto Adds stable v1 rollback execution service/messages.
crates/trident/src/server/tridentserver/services/rollback.rs Implements stable v1 rollback RPCs and preview CheckRollback; maps rollback results to streamed servicing responses.
crates/trident/src/server/tridentserver/services/mod.rs Makes rollback service module always available (not preview-only).
crates/trident/src/server/mod.rs Registers stable v1 RollbackService server; keeps preview rollback server under grpc-preview.
crates/trident/src/main.rs Adapts CLI rollback command to the new (ExitKind, ServicingType) return type.
crates/trident/src/lib.rs Updates Trident::rollback to return (ExitKind, ServicingType) and report no-op rollback explicitly.
crates/trident/src/engine/manual_rollback/mod.rs Updates rollback engine execution to return (ExitKind, ServicingType) and preserve servicing type for stage-only paths.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread crates/trident/src/server/tridentserver/services/rollback.rs
Copilot AI previously approved these changes Aug 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The functional changes appear consistent and fully wired; remaining feedback is limited to minor proto comment accuracy.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

proto/trident/v1preview/rollback_service.proto:11

  • The header comment references CompletedResponse.servicing_kind, but the message in trident/v1/servicing.proto is Completed (field servicing_kind), not CompletedResponse. This can mislead readers trying to locate the field in the proto contract.
// ServicingKind::NoneRequired for a no-op the same way every other
// servicing RPC does (see CompletedResponse.servicing_kind on
// trident.v1.ServicingResponse), trident-acl-agent no longer needs a
// separate precondition query to detect "nothing to roll back" - it reads
// servicing_kind off the RollbackStage response it already makes. This

proto/trident/v1/rollback_service.proto:11

  • The comment points readers to ServicingResponse.servicing_kind, but servicing_kind is actually a field on the Completed message inside ServicingResponse (see servicing.proto). Clarifying this avoids confusion when navigating the proto contract.
// ServicingKind::NoneRequired for a no-op (same as every other servicing
// RPC), which removed trident-acl-agent's only reason to call it - see
// ServicingResponse.servicing_kind in servicing.proto.
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@bfjelds
bfjelds requested a lite review from Copilot August 5, 2026 22:34
Copilot AI dismissed their stale review, a newer Copilot review was requested August 5, 2026 22:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The v1 promotion and return-shape changes appear consistently applied across protos, server wiring, and engine code, with no remaining mismatched call sites found in the repo.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@bfjelds
bfjelds marked this pull request as ready for review August 5, 2026 23:41
@bfjelds
bfjelds requested a review from a team as a code owner August 5, 2026 23:41
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants